Remove fastly dependency from trusted-server-core (PR 15)#635
Remove fastly dependency from trusted-server-core (PR 15)#635prk-Jr wants to merge 15 commits intofeature/edgezero-pr14-entry-point-dual-pathfrom
Conversation
BackendConfig uses fastly::backend::Backend directly, making it incompatible with the platform-portability goal. This commit: - Copies backend.rs verbatim into trusted-server-adapter-fastly, updating the one crate-internal import path - Adds url dependency to the adapter Cargo.toml - Rewires platform.rs and management_api.rs to use crate::backend - Removes pub mod backend from trusted-server-core/lib.rs - Migrates aps.rs, adserver_mock.rs, and prebid.rs off the deleted BackendConfig, using context.services.backend().ensure() for registration and a new predict_backend_name_for_url free function in integrations/mod.rs for stateless name prediction cargo check --workspace passes with zero errors.
All callers were migrated to predict_backend_name_for_url in core's integrations module. Remove the now-unused method and update the parse_origin doc comment that referenced it.
Remove crates/trusted-server-core/src/storage/ entirely (config_store.rs, secret_store.rs, mod.rs). All call sites migrated to PlatformConfigStore / PlatformSecretStore traits. Also drop the now-broken intra-doc links in trusted-server-adapter-fastly/src/platform.rs that referenced the deleted types.
Replace direct fastly::kv_store::KVStore usage in consent/kv.rs with a platform-neutral ConsentKvOps trait. The trait has four sync methods (load_entry, save_entry_with_ttl, fingerprint_unchanged, delete_entry) keeping the consent pipeline synchronous. - consent/kv.rs: add ConsentKvOps trait; replace load_consent_from_kv / save_consent_to_kv / delete_consent_from_kv with load_consent / save_consent (trait-based); remove open_store / fingerprint_unchanged private fns; drop ConsentKvMetadata / metadata_from_context (metadata API was Fastly-specific; fingerprint now lives in the body fp field); add StubKvOps + integration tests - consent/mod.rs: add kv_ops field to ConsentPipelineInput; update try_kv_fallback and try_kv_write to consume kv_ops instead of store_name; update all struct literal sites - publisher.rs: add kv_ops: Option<&dyn ConsentKvOps> parameter to handle_publisher_request; wire it into ConsentPipelineInput and use it for the SSC-revocation delete - adapter/platform.rs: add FastlyConsentKvStore wrapping fastly::kv_store::KVStore implementing ConsentKvOps via the sync API - adapter/app.rs + main.rs: open FastlyConsentKvStore from settings.consent.consent_store and pass as kv_ops to handle_publisher_request
Remove `fingerprint_unchanged` from `ConsentKvOps` trait so the common case (consent unchanged) costs one KV read instead of two. `save_consent` now loads the entry once and compares the fingerprint inline. Extract `open_consent_kv` helper in `app.rs` to deduplicate the two identical KV-open blocks. Replace bare `unwrap()` with descriptive `expect` messages in consent KV tests. Run `cargo fmt --all` to fix all whitespace issues.
Resolved five conflicts: - Deleted compat.rs (PR15 goal: remove Fastly from core) - integrations/mod.rs: kept both PR15's ensure_integration_backend_with_timeout / predict_backend_name_for_url and PR14's INTEGRATION_MAX_BODY_BYTES constant - integrations/adserver_mock.rs, aps.rs, prebid.rs: merged collect_body import (PR14) with ensure_integration_backend_with_timeout / predict_backend_name_for_url imports (PR15) into single use statements
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This PR delivers its stated goal — trusted-server-core is now fully fastly-free and the consent pipeline is abstracted behind a minimal ConsentKvOps trait. Local verification passes (fmt, clippy -D warnings, cargo test --workspace, wasm32-wasip1 release build).
Three concerns merit changes before merge: a diagnostic regression in predict_backend_name_for_url, duplication of the security-adjacent SPOOFABLE_FORWARDED_HEADERS list, and a test-coverage regression in the relocated adapter compat.rs. The remaining items are non-blocking observations and follow-up candidates.
Note: this PR targets
feature/edgezero-pr14-entry-point-dual-path, notmain; review scope is the 31-file PR-15-specific delta (+2230/-2165).
Blocking
🔧 wrench
- Diagnostic regression in
predict_backend_name_for_url— inline comment atcrates/trusted-server-core/src/integrations/mod.rs:135 SPOOFABLE_FORWARDED_HEADERSduplicated across core and adapter — inline comment atcrates/trusted-server-adapter-fastly/src/compat.rs:18- Adapter
compat.rshas zero tests after relocation (14 security-relevant tests dropped) — inline comment atcrates/trusted-server-adapter-fastly/src/compat.rs:82
Non-blocking
🤔 thinking
- KV errors logged at
debuglevel as "lookup miss" — inline comment atcrates/trusted-server-adapter-fastly/src/platform.rs:420 - Backend-name compute logic duplicated between
predict_backend_name_for_url(core) andBackendConfig::compute_name(adapter) — inline comment atcrates/trusted-server-core/src/integrations/mod.rs:130
🌱 seedling
- Redundant KV read on fallback+save path — inline comment at
crates/trusted-server-core/src/consent/kv.rs:267 - Hardcoded
certificate_check: trueinensure_integration_backend(integrations/mod.rs:69) andensure_integration_backend_with_timeout(integrations/mod.rs:114). No regression vs. pre-PR behaviour, but the siblingpredict_backend_name_for_urldoes takecertificate_check: bool— asymmetric. If any integration ever needs a self-signed upstream (test/on-prem), these helpers would need to grow that parameter.
📌 out of scope
migration_guards.rsnot extended — migration_guards.rs:27-46 only covers 12 files and a narrow regex (Request|Response|http::Method|StatusCode|mime::APPLICATION_JSON). With core'sfastlydep now removed, this guard should either broaden to\bfastly::across all core files, be replaced by a Cargo-metadata dependency-presence test (more robust, no false negatives), or be deleted as structurally redundant. Follow-up issue.FastlyConsentKvStore::opensilent failure — inline comment atcrates/trusted-server-adapter-fastly/src/platform.rs:409
📝 note
- Stale "PR 15" forward references:
- adapter-fastly/src/app.rs:150: "will be removed when
legacy_mainis deleted in PR 15". This is PR 15 andlegacy_mainwas not deleted. - adapter-fastly/src/main.rs:113: "
compat.rswill be deleted in PR 15 once this legacy path is retired". This is PR 15;compat.rswas moved (not deleted) and the legacy path was not retired.
Update comments to reference the actual future PR number or drop the promise.
- adapter-fastly/src/app.rs:150: "will be removed when
⛏ nitpick
- Unnecessary
u16type suffix — inline comment atcrates/trusted-server-core/src/integrations/mod.rs:140
CI Status
- fmt: PASS
- clippy (
-D warnings): PASS - rust tests (
cargo test --workspace): PASS (800+ tests) - wasm32-wasip1 release build: PASS
- PR-reported checks (browser integration, integration tests, prepare integration artifacts): PASS
…mpat tests - predict_backend_name_for_url now returns Result<String, Report<TrustedServerError>> instead of Option<String>; call sites use inspect_err(...).ok() to preserve the Option<String> trait interface while logging the actual failure reason - Promote SPOOFABLE_FORWARDED_HEADERS to pub in http_util; replace inline copy in compat.rs with a use import — single source of truth for the strip list - Add sanitize_fastly_forwarded_headers_strips_spoofable test (all 4 entries + Host preserved) and to_fastly_response_with_streaming_body_produces_empty_body test (pins silent-drop behaviour) to compat.rs - Change Consent KV non-ItemNotFound error log from debug "lookup miss" to warn "lookup failed" for operational visibility - Drop stale "will be removed in PR 15" forward references from app.rs and main.rs - Drop unnecessary u16 type suffixes on port literal defaults
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
- Submitted reviewer-approved findings for consent/KV handling, backend naming consistency, and Fastly wiring.
- Verdict: REQUEST_CHANGES (includes a P1 finding).
Findings moved to review body
The following approved findings were included in the review body because GitHub could not resolve their requested inline locations in the current diff:
-
🤔 Consent bootstrap logic is now duplicated across auction and publisher paths (
crates/trusted-server-core/src/auction/endpoints.rs:61)
Both handlers now repeat the same sequence: parse cookies, generate/read synthetic ID, geo lookup with identical error handling, and build ConsentPipelineInput with kv_ops. This duplication is likely to drift as the consent pipeline evolves; this PR already had to touch both call sites just to thread the new abstraction through.
Suggestion: Extract a small shared helper that prepares request-scoped consent state once and returns the ConsentContext plus any reused intermediates such as synthetic_id, geo, and cookie_jar. -
⛏ Public docs/comments still lag the new kv_ops contract (
crates/trusted-server-core/src/auction/endpoints.rs:21)
The new kv_ops parameter materially changes how consent persistence is enabled, but the public docs still describe these handlers mostly in their pre-PR form, and one consent comment still says fallback requires consent_store rather than kv_ops.
Suggestion: Update the affected doc comments to explain that consent KV fallback/write-through occurs only when a concrete kv_ops implementation is supplied, and adjust the stale internal comment in consent/mod.rs.
| /// | ||
| /// Uses the synchronous Fastly KV Store API so it is compatible with the | ||
| /// non-async consent pipeline ([`trusted_server_core::consent::build_consent_context`]). | ||
| pub struct FastlyConsentKvStore { |
There was a problem hiding this comment.
🔧 Consent revocation can be skipped after a transient KV open failure
FastlyConsentKvStore is now opened once up front and callers pass None for the whole request if that initial open() fails. That is a behavior change from the old design, where each consent KV operation opened the store independently. The regression shows up on the revoke path in publisher.rs: if KVStore::open() transiently fails at request start, the request will skip delete_entry, leaving the old consent record behind. A later cookie-less request can then fall back to stale KV consent after the user already revoked it.
Suggestion: Make the adapter consent KV wrapper lazy/open-per-operation instead of treating a single open failure as 'KV disabled for this request'. Concretely, store the KV store name in the adapter wrapper and call KVStore::open() inside load_entry / save_entry_with_ttl / delete_entry, or otherwise ensure the delete path can still retry independently.
| first_byte_timeout, | ||
| }) | ||
| .change_context(TrustedServerError::Integration { | ||
| integration: integration.to_string(), |
There was a problem hiding this comment.
🤔 Backend-name encoding is duplicated across core and Fastly adapter
predict_backend_name_for_url() in core and BackendConfig::compute_name() in the adapter both independently implement the backend naming scheme. If either side changes, auction response correlation can silently break because the predicted backend name would no longer match the name actually registered with Fastly.
Suggestion: Centralize name prediction behind the platform backend abstraction, or add a regression test that asserts both paths produce identical names for the same spec.
| .build() | ||
| } | ||
|
|
||
| /// Open the consent KV store named in `config`, returning `None` when not configured or unavailable. |
There was a problem hiding this comment.
⛏ open_consent_kv is split awkwardly across modules and call sites
The helper lives in app.rs, but main.rs only uses it for the auction path and re-implements the publisher-path open inline. It works, but it creates an odd dependency direction and makes future changes easier to miss.
Suggestion: Move the helper next to FastlyConsentKvStore in platform.rs, or make it an associated constructor, and use the same helper consistently from both entry paths.
3 participants
Summary
fastlycrate imports fromtrusted-server-core, making it fully platform-agnostic — no Fastly SDK types leak into the shared libraryConsentKvOpstrait in core so the consent pipeline stays synchronous while abstracting away the Fastly KV implementation;FastlyConsentKvStorein the adapter implements the traitcompat,backend,geo_from_fastly, config/secret stores) totrusted-server-adapter-fastlywhere it belongsChanges
crates/trusted-server-core/src/compat.rscrates/trusted-server-core/src/storage/FastlyConfigStore/FastlySecretStoreremoved (callers already on platform traits)crates/trusted-server-core/src/geo.rsgeo_from_fastly; moved to adapterplatform.rscrates/trusted-server-core/src/consent/kv.rsfastly::kv_store::KVStorewith new syncConsentKvOpstrait; fingerprint stored in bodyfpfield instead of Fastly metadatacrates/trusted-server-core/src/consent/mod.rskv_ops: Option<&dyn ConsentKvOps>throughConsentPipelineInputcrates/trusted-server-core/src/integrations/mod.rsensure_integration_backend_with_timeoutandpredict_backend_name_for_urlhelperscrates/trusted-server-core/src/integrations/{aps,adserver_mock,prebid}.rsBackendConfigto platform helperscrates/trusted-server-core/src/publisher.rskv_ops: Option<&dyn ConsentKvOps>instead of touching Fastly KV directlycrates/trusted-server-core/Cargo.tomlfastlydep; movedtokiotodev-dependencies(test-only)crates/trusted-server-adapter-fastly/src/compat.rscrates/trusted-server-adapter-fastly/src/backend.rsbackend_name_for_urlcrates/trusted-server-adapter-fastly/src/platform.rsgeo_from_fastly; addedFastlyConsentKvStoreimplementingConsentKvOpscrates/trusted-server-adapter-fastly/src/app.rsFastlyConsentKvStoreintoConsentPipelineInputcrates/trusted-server-adapter-fastly/src/main.rsFastlyConsentKvStorefor legacy path; removesuse trusted_server_core::compatcrates/trusted-server-adapter-fastly/Cargo.tomlurldep (needed by relocatedbackend.rs)Closes
Closes #496
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)